CLI: Only read stdin if there were no files supplied and not in tty mode. #567
Conversation
| @@ -106,7 +106,7 @@ module.exports = function(program) { | |||
| checker.registerDefaultRules(); | |||
| checker.configure(config); | |||
|
|
|||
| if (! process.stdin.isTTY) { | |||
| if (!process.stdin.isTTY && !args.length) { | |||
There was a problem hiding this comment.
What if an argument other than filename was specified?
There was a problem hiding this comment.
Like what? Options don't add data to the args array.
$ ./bin/jscs -
Error: Path - was not found.
at ../jscs/lib/checker.js:111:19
at Array.0 (../jscs/node_modules/vow-fs/node_modules/vow/lib/vow.js:555:56)
at Object.callFns [as _onImmediate] (../jscs/node_modules/vow-fs/node_modules/vow/lib/vow.js:1148:35)
at processImmediate [as _immediateCallback] (timers.js:336:15)Test for this we would be preferable |
| @@ -1,7 +1,5 @@ | |||
| #!/usr/bin/env node | |||
| var cli = require('../lib/cli'); | |||
| var stdin = process.stdin; | |||
| var _stdInput = []; | |||
There was a problem hiding this comment.
sad artefact, could you do it in another commit?
|
|
||
| it('should not accept piped input if files were specified (#563)', function() { | ||
| var checker = require('../lib/checker'); | ||
| var spy = sinon.spy(checker.prototype, 'checkPath'); |
There was a problem hiding this comment.
Just as in test below you need to restore the method you are spying on – see "Spying on existing methods" in here
There was a problem hiding this comment.
Restoring this method works well. Good catch. Done.
…ode or '-' supplied as first argument. Fixes jscs-dev#563
| }); | ||
|
|
||
| it('should check stdin if - was supplied as the last argument (#563)', function() { | ||
| var spy = sinon.spy(process.stdin, 'on'); |
There was a problem hiding this comment.
@markelog Restoring process.stdin.on is problematic. I'm not 100% sure why (still trying to find the cause through sinon's source), but process.stdin.on.restore only exists until the first call to process.stdin.on at https://github.com/mrjoelkemp/node-jscs/blob/fix_stdin/lib/cli.js#L117. After this call, process.stdin.on.restore is gone. Hence, adding a restore call in the promise callback throws an error.
There was a problem hiding this comment.
hm, it's already restored, by the time the test is done, so we don't need to do that ourselves
|
Why not extract https://github.com/mrjoelkemp/node-jscs/blob/3a82d8ee8a3227e7d01a68c30772b180e129ea0b/lib/cli.js#L112-L130 into a checker method Then you can just add a spy on |
|
@benesch Great tip! I'm thinking something like I think this improvement could be in a separate PR since it would require a refactor of cli.js and new tests for checker. |
|
Merged |
|
Sweet! Thanks a bunch, guys. |
Fixes #563